-
Notifications
You must be signed in to change notification settings - Fork 83
Fix malformed ActivityPub handles for email-based logins #2082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Sanitize email-based usernames (e.g., from Site Kit Google login) to prevent malformed ActivityPub handles like @[email protected]@domain.com. - Modified get_preferred_username() to detect and sanitize email logins - Added comprehensive test coverage for email username sanitization - Ensures proper webfinger format without double @ symbols Fixes #2070.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes malformed ActivityPub handles for users with email-based login names, such as those created through Site Kit Google authentication. The issue occurred when usernames containing "@" symbols resulted in double "@" symbols in ActivityPub handles.
- Modified
get_preferred_username()
to sanitize email-based usernames usingsanitize_title()
- Added comprehensive test coverage to verify proper email username sanitization
- Ensures ActivityPub handles are properly formatted without malformed double "@" symbols
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
includes/model/class-user.php | Modified get_preferred_username() to detect and sanitize email-based login names |
tests/includes/model/class-test-user.php | Added comprehensive test cases for email username sanitization scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Prefixed str_contains with a backslash to ensure the global PHP function is used, preventing potential issues with overridden or namespaced functions.
…Automattic/wordpress-activitypub into fix/email-username-sanitization
Fix PHPCS alignment warnings for variable assignments.
This comment was marked as duplicate.
This comment was marked as duplicate.
The profile URL is generated as: To maintain consistency with the ActivityPub handle, instead of replacing @ with a hyphen, the @ should be omitted entirely. 👉 Desired format: is not very “clean” as a username. It may be worth considering a redirect mechanism in the future so that such legacy usernames can point to a cleaner, newly chosen identifier. Of course, this would also require solving the name collision problem (e.g., if two users try to adopt the same “cleaned” username). 👉 In practice, this would mean: Keep the legacy handle for backwards compatibility. Allow the user/admin to register a new canonical handle. Set up a redirect/alias system so that old mentions and followers still resolve correctly. This should be addressed together with the ability for users to edit their blog profile handle. For example, mechanisms that allow arbitrary manipulation of the handle—such as using the posts page slug as the handle—should be prevented. The goal is to maintain consistency and integrity of ActivityPub handles across posts and profiles. |
Use explicit string replacement instead of sanitize_title() to ensure dots are converted to dashes as expected by tests. Also use filter_var() for proper email validation.
@obenland you use a different formatter than core, so it is not possible to run a webfinger lookup. I think core simply removes the |
includes/model/class-user.php
Outdated
|
||
// Handle cases where login is an email address (e.g., from Site Kit Google login). | ||
if ( \filter_var( $login, FILTER_VALIDATE_EMAIL ) ) { | ||
$login = \str_replace( array( '@', '.' ), '-', $login ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
$login = \str_replace( array( '@', '.' ), '-', $login ); | |
$login = \sanitize_title( $login ); |
???
includes/model/class-user.php
Outdated
|
||
// Handle cases where login is an email address (e.g., from Site Kit Google login). | ||
if ( \filter_var( $login, FILTER_VALIDATE_EMAIL ) ) { | ||
$login = \str_replace( array( '@', '.' ), '-', $login ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be even better and should be the same result:
$login = \str_replace( array( '@', '.' ), '-', $login ); | |
$login = \get_the_author_meta( 'user_nicename', $this->_id ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it comes down to the expectations we have towards what makes for an appealing handle. a49da59 moved away from sanitize_title()
to transition fediverse handles from usernamegmail-com
to username-gmail-com
.
The latter might be a bit nicer, but happy to defer if you have another preference. I don't feel strongly about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might look nicer, but it breaks the lookup! If you stick with the custom formatter you would have to also add support to the Users::get_by_* functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that's helpful!
Updated the logic in User::get_preferred_username() to use the user's nicename when the login is an email address, instead of replacing '@' and '.' with dashes. Adjusted related tests to match the new behavior.
Fixes #2070.
Proposed changes:
get_preferred_username()
method inincludes/model/class-user.php
to detect email-based logins and sanitize them usingsanitize_title()
tests/includes/model/class-test-user.php
to verify proper email username sanitization@[email protected]
instead of malformed@[email protected]@domain.com
Other information:
Testing instructions:
Setup
Test Case 1: Email-based Login from Site Kit
[email protected]
@[email protected]
@[email protected]@yourdomain.com
Test Case 2: Manual Email-based Username
[email protected]
@[email protected]
Test Case 3: Normal Username (Regression Test)
normaluser
@[email protected]
(unchanged behavior)Test Case 4: Run Unit Tests
Expected result: All tests should pass
Changelog entry
Changelog Entry Details
Significance
Type
Message
Fix malformed ActivityPub handles for users with email-based logins (e.g., from Site Kit Google authentication)